Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Expose sourceOfConfig to macro config mergers #888

Merged
merged 2 commits into from
Jul 5, 2021
Merged

Expose sourceOfConfig to macro config mergers #888

merged 2 commits into from
Jul 5, 2021

Conversation

mydea
Copy link
Contributor

@mydea mydea commented Jul 5, 2021

Currently, the merging strategy for @embroider/macros configs will just merge all given objects together. This means that the last config to be registered will "win", if a property is defined in more than one place.

Since the app's own config will be called first, this means that an app cannot overwrite configuration that is set in an addon.

This PR changes this by keeping a private __priority property on the config (which is stripped out at the end) by which we order the configs, if appropriate. The ordering works like this:

  1. setOwnConfig will always have the lowest priority
  2. setConfig called from an addon (e.g. setting config for another addon)
  3. setConfig called from the host app

The following example shows the output hierarchy:

// addon-1/index.js
setOwnConfig: {
  value1: 'default',
  value2: 'default',
  value3: 'default'
}

// addon-2/index.js
setConfig: {
  'addon-1': {
    value1: 'addon',
    value2: 'addon'
  }
}

// my-app/ember-cli-build.js
setConfig: {
  'addon-1': {
    value1: 'app'
  }
}

// Output from addon-1:
assert.deepEquals(
  getOwnConfig(), 
  { 
    value1: 'app', 
    value2: 'addon', 
    value3: 'default'
  }
);

I tried to build it in an as backwards-compatible way as possible.

Closes #885

@mydea mydea mentioned this pull request Jul 5, 2021
Merged
@ef4
Copy link
Contributor

ef4 commented Jul 5, 2021

Thanks for working on this. I know you were trying not to introduce API changes but I think it's worth having some small changes here so that custom mergers can see exactly where each config is coming from. That way, your change becomes just a change to the default merger.

  • add changes on top of yours that expose a new sourceOfConfig method to mergers
  • rebased on master

@ef4 ef4 changed the title Ensure app macro configs take precedence over addon configs Expose sourceOfConfig to macro config mergers Jul 5, 2021
@ef4 ef4 added the breaking label Jul 5, 2021
@ef4
Copy link
Contributor

ef4 commented Jul 5, 2021

I'm tagging this as breaking only because the type signature for config has changed from unknown to object (because it must be a valid WeakMap key).

@ef4 ef4 merged commit d8edf75 into embroider-build:master Jul 5, 2021
@mydea
Copy link
Contributor Author

mydea commented Jul 6, 2021

Great!
BTW while working on this, I noticed that it is currently not possible to actually set a custom merger - is that on purpose? There is no reference to useMerger anywhere in the code, as far as I see.

@ef4
Copy link
Contributor

ef4 commented Jul 6, 2021

It's possible, it's just not documented or used in any examples yet.

const { MacrosConfig } = require('@embroider/macros/node');
MacrosConfig.for(appInstance).useMerger(__filename, ...);

The appInstance is available to an addon's index.js as this._findHost() and to an app's ember-cli-build.js as the actual app local variable.

@mydea
Copy link
Contributor Author

mydea commented Jul 7, 2021

Ah, very nice - I didn't know that! Great to know if I ever get into the position again that I need some custom handling 👍

@mydea mydea deleted the fn/order-set-config branch July 22, 2021 09:52
@mydea mydea mentioned this pull request Jul 22, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
breaking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Overwriting addon macro config not working
2 participants
@mydea @ef4